-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Make default return value for NodeVisitor explicitly optional #18536
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make default return value for NodeVisitor explicitly optional #18536
Conversation
mypy/strconv.py
Outdated
| if o.metaclass: | ||
| a.insert(1, f"Metaclass({o.metaclass.accept(self)})") | ||
| inner = o.metaclass.accept(self) | ||
| a.insert(1, f"Metaclass({inner})") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and similar are a bug due to the context, I think? I would report this as an issue but I ran out of energy while doing this PR.
This comment has been minimized.
This comment has been minimized.
|
I don't really get why mypyc compiled mypy doesn't work... The stack trace points to |
|
Having to handle |
|
To debug the compiled failures, somebody would have to run the test case in a native debugger. I may look into it later. |
e2acff3 to
a06b55d
Compare
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
JukkaL
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, it's good to finally get rid of the disable_error_code hack.
Raise NotImplementedError by default in NodeVisitor visit methods.
This PR handles a TODO about making the return types from NodeVisitor explicitly optional. (I'm not personally sure this is the right way to do this given the impact, but I did as the comment said!)